⚡ general improvements #47
Conversation
… structure and readability refactor: simplify DecodeOptions and EncodeOptions constructors by removing redundant assertions feat: add _MergeFrame and enhance merge utility for better handling of complex data structures
…validation and deep nesting scenarios
…oding capabilities
…d in DecodeOptions for improved encapsulation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces recursive encoding and merging with iterative, frame-driven engines (EncodeFrame / MergeFrame); defers constructor asserts into runtime validate() for EncodeOptions/DecodeOptions (DecodeOptions adds throwOnLimitExceeded and an Expando cache); QS entrypoints call options.validate(); tests and helpers added for edge cases and deep nesting. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant QS as QS.encode
participant Validate as EncodeOptions.validate()
participant Encoder as Iterative Encoder
participant Frame as EncodeFrame Stack
participant Result as Result Accumulator
Client->>QS: encode(object, options)
QS->>Validate: options.validate()
alt Validation fails
Validate-->>QS: throw
QS-->>Client: Error
else Validation passes
QS->>Encoder: start iterative encoding
Encoder->>Frame: push root EncodeFrame
loop process frames
Frame->>Frame: prepare (keys, cycle detection)
alt primitive / fast-path
Frame->>Result: emit fragment
else nested value
Frame->>Frame: push child EncodeFrame
end
Frame->>Result: onResult aggregates fragments
end
Result-->>QS: aggregated query string
QS-->>Client: return string
end
sequenceDiagram
participant Client as Client Code
participant QS as QS.decode
participant Options as DecodeOptions
participant Validator as Options.validate()
participant Parser as Tokeniser/Parser
participant Decoder as Iterative Merge Engine
participant MergeFrame as MergeFrame Stack
participant Result as Decoded Object
Client->>QS: decode(qs, options)
QS->>Options: options.validate()
alt Validation fails
Options-->>QS: throw
QS-->>Client: Error
else Validation OK / cached
QS->>Parser: parse tokens
Parser->>Decoder: walk tokens
Decoder->>MergeFrame: push root MergeFrame
loop process frames
MergeFrame->>MergeFrame: normalize shape, iterate keys/indices
alt nested map/list
MergeFrame->>MergeFrame: spawn child MergeFrame
else leaf value
MergeFrame->>Result: set value
end
MergeFrame->>Decoder: onResult propagate
end
Decoder-->>QS: decoded object
QS-->>Client: return object
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the core encoding and decoding logic to improve robustness and maintainability by replacing recursion with iterative stack-based approaches, while also moving option validation from constructor-time to usage-time.
Changes:
- Replaced recursive encoding with an iterative stack-based approach using
_EncodeFramefor better cycle detection and to prevent stack overflow on deeply nested structures. - Replaced recursive merging in
Utils.mergewith an iterative stack-based approach using_MergeFrameto handle deep maps without stack overflow. - Moved
DecodeOptionsandEncodeOptionsvalidation from constructor assertions to dedicated validation methods called at usage time (QS.decode/QS.encode), providing clearer error handling.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/src/extensions/encode.dart |
Replaced recursive _encode with iterative stack-based traversal using _EncodeFrame; removed _sentinel WeakMap anchor |
lib/src/models/encode_frame.dart |
Introduced new _EncodeFrame class to track encoder state across iterative traversal |
lib/src/utils.dart |
Replaced recursive Utils.merge with iterative stack-based approach using _MergeFrame and _MergePhase; improved ByteBuffer charset handling |
lib/src/models/decode_options.dart |
Moved validation from constructor assertions to validate() method; added Expando cache for validated instances; added throwOnLimitExceeded to copyWith and toString |
lib/src/models/encode_options.dart |
Removed constructor assertions for charset and filter validation |
lib/src/qs.dart |
Added validation calls before encoding/decoding; introduced _validateEncodeOptions helper; added encode_frame.dart part |
test/unit/utils_test.dart |
Added test for deep map merging without stack overflow |
test/unit/encode_edge_cases_test.dart |
Added test for deep nesting encoding without stack overflow |
test/unit/models/decode_options_test.dart |
Updated tests to validate options at usage time rather than construction; added runtime validation tests; added _FakeEncoding helper |
test/unit/models/encode_options_test.dart |
Added runtime validation tests with _FakeEncoding helper |
test/unit/decode_test.dart |
Made DecodeOptions constructors const where possible; updated error expectation matchers |
test/unit/uri_extension_test.dart |
Updated error expectation matchers to include ArgumentError and StateError |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
- Coverage 97.90% 97.27% -0.64%
==========================================
Files 14 16 +2
Lines 954 1136 +182
==========================================
+ Hits 934 1105 +171
- Misses 20 31 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…r usage of decodeKey/decodeValue
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/models/decode_options.dart (1)
53-78:⚠️ Potential issue | 🟠 MajorAdd debug-time asserts to the const constructor for option invariants.
The current implementation defers all validation to the
validate()method, allowing invalid option combinations to be constructed without immediate feedback. Project guidance requires option constructors to validate invariants via assert statements. Dart const constructors support asserts in the initialiser list—when invoked withconst, a failing assert produces a compile-time error for that instantiation.Add asserts for:
charsetmust be UTF-8 or Latin-1decodeDotInKeys: truerequiresallowDotsto be truthyparameterLimitmust be positiveTests that intentionally construct invalid options will need to use non-const instances (removing
constkeyword) to allow the asserts to fail at runtime during decode.♻️ Proposed asserts for initialiser list
}) : allowDots = allowDots ?? (decodeDotInKeys ?? false), decodeDotInKeys = decodeDotInKeys ?? false, _decoder = decoder, - _legacyDecoder = legacyDecoder; + _legacyDecoder = legacyDecoder, + assert( + charset == utf8 || charset == latin1, + 'Invalid charset', + ), + assert( + !(decodeDotInKeys ?? false) || + (allowDots ?? (decodeDotInKeys ?? false)), + 'decodeDotInKeys requires allowDots to be true', + ), + assert( + parameterLimit > 0, + 'Parameter limit must be a positive number.', + );
🤖 Fix all issues with AI agents
In `@lib/src/utils.dart`:
- Around line 165-173: The normalization branch that runs when
frame.options?.parseLists == false returns a SplayTreeMap with int keys and is
skipped by the post-list-merge path, so change both places (the branch around
frame.options?.parseLists and the similar code at the other location) to: after
list merges run normalization that converts keys to string (use
entry.key.toString()) and filter out Undefined values, and ensure frame.onResult
is called with a Map<String, dynamic> (not SplayTreeMap<int, dynamic>) so
array-shaped results never contain Undefined; update the code around target_,
stack.removeLast(), and frame.onResult to produce the normalized Map<String,
dynamic> in both branches.
- Around line 149-201: The hole-handling branch that runs when
currentTarget.any((el) => el is Undefined) currently executes before the
list-of-maps merge logic, causing lists-of-maps with holes to be flattened
instead of merged by index; compute or check whether both sides are map-lists
(e.g., bool targetMaps = currentTarget.every((el) => el is Map || el is
Undefined); bool sourceMaps = currentSource is Iterable ?
currentSource.every((el) => el is Map || el is Undefined) : false) and guard the
hole branch so it only runs when NOT (targetMaps && sourceMaps); alternatively
move the targetMaps/sourceMaps computation above the Undefined check and skip
the early hole shortcut when targetMaps && sourceMaps so the code falls through
to the logic that sets frame.indexedTarget, frame.sourceList, frame.targetIsSet,
frame.listIndex and frame.phase = _MergePhase.listIter.
🧹 Nitpick comments (1)
lib/src/utils.dart (1)
15-18: Use PascalCase for_MergePhasevalues.The enum values
start,mapIter, andlistIterdon’t follow the PascalCase rule for enum members. Please rename them (and update references) to match the project convention. As per coding guidelines: Use PascalCase for enum member names.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/src/enums/merge_phase.dart`:
- Around line 7-15: Rename the enum members in _MergePhase from start, mapIter,
listIter to PascalCase Start, MapIter, ListIter and update all call sites
referencing those members (there are 7 references across the codebase) so they
use the new names; specifically modify the definition in _MergePhase and then
update usages in the functions and classes in utils (e.g., any code referencing
_MergePhase.start/_MergePhase.mapIter/_MergePhase.listIter) and in
MergeFrame-related code (e.g., any references in the MergeFrame class) to the
corresponding PascalCase names.
…s for better error handling
…hase for improved clarity
|
@codex review |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9d8982536
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9d8982536
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…and merging semantics
… for parameterLimit
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 participant
This pull request introduces significant improvements to the
qs_dartpackage's encoding and decoding logic, focusing on making the encoder iterative (stack-based) instead of recursive, improving cycle detection, and strengthening option validation. The changes also clarify documentation and error handling for decode options.Key changes include:
Encoding Refactor and Improvements
encode.dartfrom a recursive implementation to an iterative, stack-based approach using an explicit stack ofEncodeFrameobjects. This removes recursion limits, improves cycle detection, and makes the code more maintainable and robust. [1] [2] [3] [4] [5] [6]MergePhaseinmerge_phase.dartto drive state machines for iterative merges, supporting future improvements and clarifying merge phases.Decode Options Validation and Documentation
DecodeOptions, clarifying that invalid combinations (such asdecodeDotInKeys: truewithallowDots: false) will throw when validated or used, not just at construction. [1] [2] [3]validate()method toDecodeOptionsthat enforces invariants at runtime, ensuring options are correct before decoding. This method is called in decode entry points and direct decoder calls. [1] [2]throwOnLimitExceededoption toDecodeOptions.copyWith, allowing more granular control over decode behavior.These changes collectively improve the reliability, maintainability, and clarity of the encoding/decoding logic in the package.